-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: 1257 - Load ocrd tool json locally #1260
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Straightforward.
But then perhaps we should make sure that an ocrd-all-tool.json
does always exist in any given core installation, independent of our ocrd_all recipes. We could create a default ocrd-all-tool.json
(with just the ocrd-dummy processor) as package data. (Which the ocrd_all recipes would merely overwrite in turn.)
Oh, the integration tests fail here:
With no further messages, I suppose this will be hard to reproduce, right? |
The reason behind this is the missing |
Oh, of course! Then I suggest modifying this PR to include the default |
Done. Right before your comment, I have also added a download if it is missing. |
Ah, thanks. I was just thinking about generating this dynamically – basically a
Ok, so we have both mechanisms. I wonder if it wouldn't be better to not support the download strategy anymore... BTW, now a new CI failure appeared: we get interference from some new paramiko version's stderr chatter about coming deprecations (completely irrevelant to us). Perhaps we can suppress this within our ocrd_logging.conf? |
Setting |
Indeed – they use the
It will depend on the venv's (or OS'?) cryptography version.
Right – testing for zero stderr was not a good pattern in the first place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Pinning cryptography to an older version is problematic but if paramiko/paramiko#2421 is merged soon, there will probably a proper solution soon.
Ready for merge?
Co-authored-by: Konstantin Baierer <[email protected]>
A fix for #1257.